Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Web3 in favor of ethersjs #1083

Merged
merged 10 commits into from
Jul 5, 2023
Merged

Remove Web3 in favor of ethersjs #1083

merged 10 commits into from
Jul 5, 2023

Conversation

kuzdogan
Copy link
Member

@kuzdogan kuzdogan commented Jun 28, 2023 •
edited
Loading

Draft PR

To keep consistency we should remove all Web3.js dependencies and instead use ethers.js.

View in Huly HI-548

@codecov
Copy link

codecov bot commented Jun 30, 2023 •
edited
Loading

Codecov Report

Patch coverage: 80.00% and project coverage change: -4.85 ⚠️

Comparison is base (42828a3) 78.88% compared to head (44925c9) 74.03%.

❗ Current head 44925c9 differs from pull request most recent head d4397c7. Consider uploading reports for the commit d4397c7 to get more accurate results

Additional details and impacted files 
@@             Coverage Diff             @@
##           staging    #1083      +/-   ##
===========================================
- Coverage    78.88%   74.03%   -4.85%     
===========================================
  Files           56       66      +10     
  Lines         1539     2392     +853     
  Branches       271      441     +170     
===========================================
+ Hits          1214     1771     +557     
- Misses         189      391     +202     
- Partials       136      230      +94
Flag Coverage Δ
lib-sourcify 65.73% <ø> (?)
server 78.68% <80.00%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ommon/SourcifyEventManager/SourcifyEventManager.ts 100.00% <ø> (ø)
src/monitor/monitor.ts 81.45% <72.72%> (-3.71%) ⬇️
src/monitor/pending-contract.ts 82.60% <100.00%> (ø)
src/server/common.ts 68.75% <100.00%> (-0.95%) ⬇️
...er/controllers/verification/verification.common.ts 80.92% <100.00%> (ø)
src/server/server.ts 75.22% <100.00%> (ø)
src/server/services/RepositoryService.ts 73.46% <100.00%> (ø)

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kuzdogan kuzdogan force-pushed the web3-to-ethersjs branch from 706e6d9 to 8146aa0 Compare July 3, 2023 10:26
kuzdogan added 8 commits July 3, 2023 12:29
@kuzdogan
Remove Web3 in lib-sourcify
e92dca0
@kuzdogan
Refactor malicious creationBytecode test
5d36ed9 
The test was failing because of an ethers.js error: ethers-io/ethers.js#4182

Remove the unnecessary contract deployment and explain the test
in detail.

The test code and it's title was not really understandable becuase it
was mostly copy-pasted from the above test.
@kuzdogan
Update contract-call-decoder package-lock
e730091
@kuzdogan
Fix .only in tests
238e374
@kuzdogan
Remove web3js in server and monitor
3358265
@kuzdogan
Remove web3js in monitor tests, refactor monitor
56996ae 
Removes the webjs dependency in favor of ethers.js in monitor tests, as
well as in monitor.

Change ChainMonitor state variables chainId etc. into a single
SourcifyChain.

Monitor optionally takes a SourcifyChain[] as a parameter to initialize,
otherwise gets the monitored chains from sourcify-chains.ts

Using SourcifyChain in Monitor causes chainId to be a number. Change the
variable type in places and in SourcifyEventManager accordingly.
@kuzdogan
Delete unused test/utils.js
add6d08
@kuzdogan
Remove web3js from server tests
d1f658a
@kuzdogan kuzdogan force-pushed the web3-to-ethersjs branch from 8146aa0 to d1f658a Compare July 3, 2023 10:29
@kuzdogan kuzdogan marked this pull request as ready for review July 3, 2023 14:11
@kuzdogan kuzdogan requested a review from marcocastignoli July 3, 2023 14:11
@kuzdogan
Copy link
Member Author

kuzdogan commented Jul 3, 2023

Is codecov failing because of insufficient coverage? @marcocastignoli

@marcocastignoli
Copy link
Member

Is codecov failing because of insufficient coverage? @marcocastignoli

in lib-sourcify we have a minimum that is:

    "lines": 75,
    "statements": 75,
    "functions": 75,
    "branches": 60

marcocastignoli
marcocastignoli reviewed Jul 3, 2023
View reviewed changes
Copy link
Member

@marcocastignoli marcocastignoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run the monitor test? Just a few comments, my only real concern is about the getAddress that changes the behavior of the previous function.

src/server/controllers/verification/verification.common.ts Show resolved Hide resolved
src/common/SourcifyEventManager/SourcifyEventManager.ts Outdated Show resolved Hide resolved
src/server/services/RepositoryService.ts Show resolved Hide resolved
@kuzdogan
Copy link
Member Author

kuzdogan commented Jul 3, 2023

The node-16 tests also cover monitor tests no? @marcocastignoli

@marcocastignoli
Copy link
Member

The node-16 tests also cover monitor tests no? @marcocastignoli

oh yes, I totally forgot it 😆

@kuzdogan kuzdogan force-pushed the web3-to-ethersjs branch from c2cba54 to d4397c7 Compare July 5, 2023 12:43
@kuzdogan
Copy link
Member Author

kuzdogan commented Jul 5, 2023 •
edited
Loading

Force pushed to clean the code from node-v16 the fix for should run getCreatorTx with regex. Will move that to a new branch.

Please have a short look at the last commit @marcocastignoli
d4397c7

@kuzdogan kuzdogan requested a review from marcocastignoli July 5, 2023 12:45
@kuzdogan kuzdogan merged commit a7b349b into staging Jul 5, 2023
@kuzdogan kuzdogan deleted the web3-to-ethersjs branch December 19, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcocastignoli marcocastignoli marcocastignoli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kuzdogan @marcocastignoli